Fix phpstan/phpstan#14214: coalesce is slow with lots of variables#5113
Closed
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Closed
Fix phpstan/phpstan#14214: coalesce is slow with lots of variables#5113phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Conversation
- Avoided expensive $scope->getType($expr->right) call when the right side of a coalesce is itself a Coalesce expression - For right-associative chains like $a ?? $b ?? $c ?? $d, getType on the nested chain recursively evaluated the entire remaining chain at each level, causing O(N^2) complexity - Since a nested Coalesce expression is never an explicit NeverType, the getType call can be safely skipped for Coalesce right operands - New regression test in tests/PHPStan/Analyser/data/bug-14214.php Closes phpstan/phpstan#14214
| $rightScope = $scope->filterByFalseyValue($expr); | ||
| $rightResult = $this->processExprNode($stmt, $expr->right, $rightScope, $storage, $nodeCallback, $context->enterDeep()); | ||
| $rightExprType = $scope->getType($expr->right); | ||
| $rightExprType = $expr->right instanceof Coalesce ? null : $scope->getType($expr->right); |
Contributor
There was a problem hiding this comment.
This seems really specific,
- this at least need a comment
- and maybe they is more case to skip ?
Member
There was a problem hiding this comment.
It could be better maybe but it solves the problem. I recently did the same fix with Ternary.
Contributor
|
I can no longer reproduce slowness |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Coalesce chains with many variables (e.g.,
$x0 ?? $x1 ?? ... ?? $x16) were extremely slow to analyse (15-20 seconds reported). This is the same class of issue as phpstan/phpstan#14207 (fixed for BooleanAnd/BooleanOr), but for the null coalesce operator.Changes
src/Analyser/NodeScopeResolver.php(line ~3695): Skip the expensive$scope->getType($expr->right)call when the right operand of a coalesce is itself aCoalesceexpression. The call is only needed to check for explicitNeverType(e.g.,$x ?? throw new Exception()), which cannot occur for nested Coalesce expressions.tests/PHPStan/Analyser/data/bug-14214.phpwith a 17-variable coalesce chain andassertTypeverification.testBug14214intests/PHPStan/Analyser/AnalyserIntegrationTest.php.Root cause
The coalesce operator (
??) is right-associative, so$a ?? $b ?? $cparses as$a ?? ($b ?? $c). In theNodeScopeResolvercoalesce handler,$scope->getType($expr->right)was called to check if the right side is an explicitNeverType. For a nested Coalesce right operand,getTypecallsgetCoalesceType, which recursively evaluates the entire remaining chain — O(N) work per level. Since this happens at each of the N nesting levels, total complexity was O(N^2).The fix skips this
getTypecall when the right operand is a Coalesce node. A nested Coalesce expression can never be an explicitNeverType(it always includes the possibility of returning a non-never left operand value), so the check is unnecessary for this case.Test
The regression test
tests/PHPStan/Analyser/data/bug-14214.phpreproduces the original issue with 17 nullable variables chained via??, and verifies the inferred type isint<0, 33>|null. Before the fix, this test took ~2.2s; after the fix, it completes in ~0.7s.Fixes phpstan/phpstan#14214